Repository navigation
Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 13 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready to merge once re-approved (the first rebase force-push dismissed the earlier approval). Requested in the mordant baseline sweep (the 32-count
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical return-type change from bool to bun_io::Result<()> on three frame serializers, with no behavior change.
What was reviewed:
- Confirmed all ~31
let _ = ….write(…)call sites inh2_frame_parser.rsstill discard the result identically; the one reader inStream::flush_queuenow appends.is_ok()to recover the same bool. - Checked for any
if x.write(…)/ boolean-condition uses that would break with aResultreturn — none found. mordant-baseline.tomlregeneration drops the resolved 32-count entry plus three empty crate tables; no unrelated entries touched.
Extended reasoning...
Overview
This PR changes three HTTP/2 frame serializer methods (FrameHeader::write, StreamPriority::write, UInt31WithReserved::write) in src/runtime/api/bun/h2_frame_parser.rs to return bun_io::Result<()> directly from writer.write_all(..) instead of collapsing it to a bool via .is_ok(). The one call site that consumed the bool (Stream::flush_queue, line 1743) now applies .is_ok() at the call site. mordant-baseline.toml is regenerated to drop the corresponding error_collapsed_to_bool lint entry and three empty crate tables left by prior hand edits.
Security risks
None. This is a return-type refactor on internal wire-serialization helpers with no change to what bytes are written, when, or how errors propagate. No user input handling, auth, crypto, or permissions are touched.
Level of scrutiny
Low. The change is purely mechanical: dropping .is_ok() from three method bodies and adding it back at the single call site that needs a bool. I grepped the file for every .write( call on these types — all remaining ~30 sites use let _ =, which discards a Result<()> exactly as it discarded a bool, so they compile and behave unchanged. I also searched for any if …write(…) conditionals or other bool-typed uses that would break; there are none. The sibling branches in flush_queue already compute writer.write_all(..).is_ok() directly, so the updated site now matches them.
Other factors
The PR description explains why discarding is intentional (fixed-size stack buffers cannot fail; DirectWriterStruct errors mean buffered-for-later, not lost data), which matches the existing pattern throughout the file. FullSettingsPayload::write was intentionally left unchanged as dead code under the file's transitional #[allow(dead_code)]. The author ran the http2 test suite (447 tests) on the debug build. The baseline file diff is a clean regeneration — only the resolved entry and empty tables are removed, nothing else altered.
|
Updated 6:13 PM PT - Aug 20th, 2026
❌ @robobun, your commit 1f653a4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39262That installs a local version of the PR into your bun-39262 --bun |
84f72c2 to
2140c4d
Compare
There was a problem hiding this comment.
LGTM — mechanical signature change with no behavior difference.
What was reviewed:
- The three
writemethods now returnbun_io::Result<()>instead of collapsing to.is_ok(); all 27 grepped call sites uselet _ =(discardsResultthe same asbool) except the one inStream::flush_queue, which correctly appends.is_ok()to preserve the backpressure-flag semantics. - Confirmed no other call site read the old bool return value, so nothing else needs
.is_ok(). mordant-baseline.tomlchanges are the removed 32-count entry plus three empty-table drops from regeneration; no unrelated counts touched.
Extended reasoning...
Overview
This PR changes the return type of three HTTP/2 frame-serializer helpers (FrameHeader::write, StreamPriority::write, UInt31WithReserved::write) in src/runtime/api/bun/h2_frame_parser.rs from bool to bun_io::Result<()>, dropping the trailing .is_ok() so they return the underlying write_all result directly. One call site (Stream::flush_queue) that consumed the bool now calls .is_ok() itself; all others already discard via let _ =. mordant-baseline.toml is regenerated to drop the 32-count error_collapsed_to_bool entry and three empty crate tables.
Security risks
None. This is a return-type refactor on internal wire serializers with no change to what bytes are written, no new inputs, and no error-path behavior change.
Level of scrutiny
Low. The diff is a lint-driven cleanup requested in the mordant baseline sweep. Each of the three method bodies is a single write_all call whose result was previously mapped through .is_ok(); now it's returned as-is. let _ = expr discards a #[must_use] Result exactly as it discarded a bool, so the ~30 discard sites compile and behave identically. I grepped every .write(&mut ...) call in the file and confirmed the flush_queue site is the only one that consumed the return, and it now maps back to the same bool via .is_ok().
Other factors
- The PR notes
FullSettingsPayload::writehas the same shape but is dead code under the file-level#[allow(dead_code)]; leaving it out of this change is reasonable and it isn't called anywhere. - The http2 test suites (359 + 88 tests) were run on the debug build per the PR body/status comment; no test changes are expected since there is no observable behavior difference.
- Already approved by alii per the robobun status comment; no outstanding reviewer feedback and no prior claude[bot] review on this PR.
There was a problem hiding this comment.
LGTM — mechanical signature change (bool → bun_io::Result<()>) with no behavior difference.
What was reviewed:
- Traced all 32 call sites: 31 are
let _ = ...(discard either way); the one reader inStream::flush_queuenow appends.is_ok(), producing the identical bool it got before. - Confirmed
let _ =on aResultcompiles without#[must_use]warnings, so the discarding sites are unaffected. - Baseline regen drops the 32-count
error_collapsed_to_boolentry and three empty crate tables — matches whatrust:mordant:baselineemits (never writes empty tables).
Extended reasoning...
Overview
The PR changes the return type of three HTTP/2 frame serializer methods in src/runtime/api/bun/h2_frame_parser.rs — FrameHeader::write, StreamPriority::write, and UInt31WithReserved::write — from bool to bun_io::Result<()>. Each method wraps a single writer.write_all(..) call; previously they collapsed the result with .is_ok(), now they propagate it directly. The one call site that consumed the bool (Stream::flush_queue, using it as a backpressure flag) appends .is_ok() to preserve identical semantics. The other 31 call sites use let _ = ..., which discards either a bool or a Result<()> identically and suppresses #[must_use]. mordant-baseline.toml is regenerated, removing the 32-count error_collapsed_to_bool entry for this file plus three empty crate table headers left over from earlier hand edits.
Security risks
None. No untrusted input handling, no allocation, no control-flow changes. The bytes written to the wire are byte-for-byte identical; only the Rust return type of three internal helpers changes.
Level of scrutiny
Low. This is a type-only refactor targeting a repo lint baseline entry. I grepped every .write( call site against these three types in the file (32 hits, matching the baseline count) and confirmed each is either let _ = or the single flush_queue site that now has .is_ok(). The frames_sent.set(frames_sent.get() + 1) side effect in FrameHeader::write still runs unconditionally before the tail write_all, so the counter behavior is unchanged too. The sibling branches of the 'brk block in flush_queue already compute their bool via writer.write_all(..).is_ok(), so this brings the zero-payload branch to the same shape.
Other factors
The PR description reports the full http2 test suites pass on the debug build after rebase, bun run rust:mordant:baseline reproduces the committed file exactly, and cargo fmt --check is clean. The one red CI check (Source lints / jsresult-swallow.test.ts) is stated to fail identically on main and does not touch lines in this diff. No CODEOWNERS entry covers the touched paths. There are no outstanding human review comments; the CodeRabbit comment is a rate-limit notice with no content. The description notes an earlier approval was dismissed by the rebase force-push; the rebase conflict was trivial (a deleted from_bytes adjacent to the changed write signature), and I re-verified the post-rebase state directly.
… of a bool FrameHeader::write, StreamPriority::write and UInt31WithReserved::write wrapped writer.write_all(..) and collapsed its error into a bool. All but one caller discard the value; the one that reads it (Stream::flush_queue) wants a backpressure flag, which it now derives with .is_ok() like the sibling branches that call write_all directly. No behavior change. Regenerates mordant-baseline.toml, which drops the 32 error_collapsed_to_bool entries for this file and the empty crate tables the file was carrying.
86e39e1 to
1f653a4
Compare
There was a problem hiding this comment.
LGTM — mechanical return-type change with no behavior difference.
What was reviewed:
- The three
writebodies: only the trailing.is_ok()is dropped; thewrite_allcall and byte layout are unchanged. - All call sites in
h2_frame_parser.rs: every one islet _ = …exceptflush_queue, which now appends.is_ok()— same bool as before. mordant-baseline.toml: only the 32-count entry and two empty crate headers are removed; no other counts changed.
Extended reasoning...
Overview
The PR changes three tiny frame-serializer methods in src/runtime/api/bun/h2_frame_parser.rs (FrameHeader::write, StreamPriority::write, UInt31WithReserved::write) to return bun_io::Result<()> instead of bool, by deleting the trailing .is_ok() on the single write_all each wraps. The one call site that read the return value (Stream::flush_queue) gets .is_ok() appended so it computes the same bool. mordant-baseline.toml is regenerated, dropping the error_collapsed_to_bool:…h2_frame_parser.rs = 32 entry and two empty crate table headers.
Security risks
None. This is a return-type refactor in outbound HTTP/2 frame serialization; no input parsing, allocation sizing, auth, or crypto is touched, and no new error path is introduced.
Level of scrutiny
Low. The bytes written are byte-for-byte identical (only the return expression changed), and I grepped every call site in the file: all are let _ = … discards, which discard a Result exactly as they discarded the bool, so those sites compile and behave identically. The single non-discarding site now maps the same write_all outcome to the same bool via .is_ok(). This is the definition of a behavior-preserving lint cleanup.
Other factors
The full node-http2 test suite plus the h2 conformance/continuation/padding/rehash tests were run on the debug build and pass. The mordant CI job is the regression guard for the pattern returning. The baseline diff is minimal (one entry removed, two empty section headers dropped by the regenerator) and touches no other counts. No outstanding human review comments; the only timeline entries are bot status/rate-limit notices.
Problem
FrameHeader::write,StreamPriority::writeandUInt31WithReserved::writeinsrc/runtime/api/bun/h2_frame_parser.rseach wrap onewriter.write_all(..)call and return.is_ok(), turning the writer'sbun_core::Errorinto a bool.let _ = frame.write(..)). The one reader,Stream::flush_queue, uses it as a backpressure flag, the same thing its sibling branches compute withwriter.write_all(..).is_ok()directly.error_collapsed_to_bool:src/runtime/api/bun/h2_frame_parser.rs = 32entry inmordant-baseline.toml.Fix
bun_io::Result<()>they get fromwrite_all, matching the signature of what they wrap (and ofDirectWriterStruct::write_all/write_paddedfurther down the file). Thelet _ =call sites compile unchanged;flush_queueappends.is_ok().flush_queue) maps the samewrite_alloutcome to the same bool. Discarding is intentional at those sites: the writers are either a stackFixedBufferStreamsized for the frame, which cannot fail, orDirectWriterStruct, whoseErrmeans the bytes were buffered for a later flush, and the rest of the frame still has to be written in order.mordant-baseline.tomlregenerated withbun run rust:mordant:baseline: the 32-count entry is gone. The regeneration also drops two empty crate tables ([bun_ast],[bun_sql_jsc]) that earlier hand edits had left in the file (the generated file never contains empty tables);bun run rust:mordantthen reports nothing over the baseline.test/js/node/http2/node-http2.test.js(361 pass, 6 skip; thedescribe.concurrentdetached-payload block times out intermittently at its 5 s default on debug builds, the pre-existing flake test(http2): give the detached-payload subprocess cases a debug-scaled timeout #38078 covers, and does so without this change too), plush2-conformance.test.ts,node-http2-continuation.test.ts,node-http2-invalid-padding.test.tsandnode-http2-streams-rehash.test.ts(88 pass).cargo fmt --checkclean. Buildkite build 100195 ran this same diff on the previous rebase and passed fully.mordantCI job fails if awrite_allresult is collapsed to a bool in this file again.Background
bun_io::Writesink.bun_io::FixedBufferStreamover a stack array sized to the frame, andDirectWriterStruct, which forwards toH2FrameParser::write. That function returns false when the socket did not take the bytes; they are kept in the parser's write buffer and flushed later, soErrfrom these serializers means backpressure, not lost data.mordant-baseline.tomlis the per-(lint, file) count of accepted findings for the repo's Rust lint pack; CI fails only on findings above it, and fixing some means regenerating the file so the count stays down.Rebase note
Rebased onto main after #39448 removed the dead inbound half of this file. The only conflict was the deleted
UInt31WithReserved::from_bytessitting next to the changedwritesignature; resolved by keeping the deletion. #39448 also removed the unusedFullSettingsPayload::write, which an earlier version of this description mentioned leaving alone. The baseline was regenerated again on the rebased tree and came out identical to the merged result.Rebased a second time when other sweep PRs changed
mordant-baseline.toml(the only conflicting file). Resolved by taking main's copy and runningbun run rust:mordant:baselineagain on the rebased tree; the result is main's current baseline minus this file's entry and the two empty tables. The source change applied cleanly.